-
Notifications
You must be signed in to change notification settings - Fork 6.8k
Conversation
@huangzhiyuan please verify the performance in #12303 and other tests from your local. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any specific test case for this change is added?
offsets[i] = s; | ||
} | ||
auto in_mem = in.GetMKLDNNData(); | ||
auto in_mem_pd = in_mem->get_primitive_desc(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please merge in_mem and in_mem_pd into one line as out_mem_pd in case the in_mem doesn't use in here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just for the overall code style, I can also modify it if you insist.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the "overall code style"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have merge im_mem and im_mem_pd into one line in the new commit.
@pengzhao-intel No specific test case for this change should be added, the current test case is enough to cover. Thanks for your review :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the improvements!
LGTM
offsets[i] = s; | ||
} | ||
auto in_mem = in.GetMKLDNNData(); | ||
auto in_mem_pd = in_mem->get_primitive_desc(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the "overall code style"?
static MX_THREAD_LOCAL std::unordered_map<MKLDNNSliceSignature, MKLDNNSliceFwd, OpHash> fwds; | ||
#endif | ||
MKLDNNSliceSignature key(param); | ||
key.AddSign(in_data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to have is_train
and out_data
into the key?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In_data is enough for the key to cache. And I have put is_train and out_data into the key in new commit.
src/operator/tensor/matrix_op-inl.h
Outdated
|
||
if (in_stype == kDefaultStorage) { | ||
#if MXNET_USE_MKLDNN == 1 | ||
if (dev_mask == Context::kCPU && MKLDNNEnvSet() && SupportMKLDNNSlice(param)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
indent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the fix. Let's wait for CI passing.
Closes #12303 . |
* add mkldnn slice * fix lint * fix lint * mv SliceEx to matrix_op.cc * fix lint * optimize dispatch_mode * retrigger ci * fix indent
* add mkldnn slice * fix lint * fix lint * mv SliceEx to matrix_op.cc * fix lint * optimize dispatch_mode * retrigger ci * fix indent
Description
Add mkldnn implement for slice OP
Checklist
Essentials
Please feel free to remove inapplicable items for your PR.
Changes
test_slice has passed
@pengzhao-intel , @ZhennanQin , @TaoLv